src/layout/ImageUpload/imageUploadUtils.ts (4)

34-45: Use natural image dimensions; handle zero to avoid Infinity/NaN.

This issue was flagged in a previous review: HTMLImageElement.width/height returns CSS pixels, but pixel-accurate canvas calculations require naturalWidth/naturalHeight. Additionally, guard against zero dimensions to prevent Infinity/NaN in clamp calculations.

Refer to the previous review comment for the complete fix.

---

54-61: Use natural image dimensions; handle zero to avoid Infinity/NaN.

This function has the same issue flagged in the previous review: use naturalWidth/naturalHeight instead of width/height for accurate canvas calculations, and guard against zero dimensions.

Refer to the previous review comment for the complete fix.

---

98-115: Guard zoom math against invalid ranges; clamp and avoid NaN/∞.

This issue was flagged in a previous review: Math.log with non-positive minZoom/maxZoom/value yields -Infinity/NaN. The code needs input validation and output clamping to keep the slider domain stable.

Refer to the previous review comment for the complete fix.

---

118-119: Use natural image dimensions; handle zero to avoid Infinity/NaN.

This function has the same issue flagged in the previous review: use naturalWidth/naturalHeight instead of width/height, and guard against zero dimensions to prevent returning Infinity.

Refer to the previous review comment for the complete fix.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/layout/ImageUpload/ImageCropper.tsx (2)

99-114: Clean up pending Image/FileReader to avoid leaks and stale callbacks
When replacing files or unmounting mid‑load, the previous Image can continue loading. Track and cancel previous loaders; guard onload against stale updates.

+  const loadingImageRef = useRef<HTMLImageElement | null>(null);
+  const fileReaderRef = useRef<FileReader | null>(null);
   const handleFileUpload = (file: File) => {
     const validationErrors = validateFile(file);
     setValidationErrors(validationErrors);
     if (validationErrors.length > 0) {
       return;
     }
-
-    const reader = new FileReader();
+    // Cleanup any in-flight loaders
+    if (loadingImageRef.current) {
+      loadingImageRef.current.onload = null;
+      loadingImageRef.current.src = '';
+      loadingImageRef.current = null;
+    }
+    if (fileReaderRef.current && 'readyState' in fileReaderRef.current) {
+      try { fileReaderRef.current.abort?.(); } catch {}
+    }
+    const reader = new FileReader();
+    fileReaderRef.current = reader;
     reader.onload = (event) => {
       const result = event.target?.result;
 
       if (typeof result === 'string') {
         const img = new Image();
+        loadingImageRef.current = img;
         img.id = file.name;
         imageTypeRef.current = file.type;
         img.onload = () => {
-          updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img });
+          if (loadingImageRef.current === img) {
+            updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img });
+            loadingImageRef.current = null;
+          }
         };
         img.src = result;
       }
     };
     reader.readAsDataURL(file);
   };
+  React.useEffect(() => () => {
+    if (loadingImageRef.current) {
+      loadingImageRef.current.onload = null;
+      loadingImageRef.current.src = '';
+    }
+    try { fileReaderRef.current?.abort?.(); } catch {}
+  }, []);

---

136-144: Handle canvas.toBlob failure and report to user

If toBlob returns null, provide feedback; also wrap File creation to catch exceptions.

-    cropCanvas.toBlob((blob) => {
-      if (!blob) {
-        return;
-      }
-      const fileName = img.id;
-      const imageFile = new File([blob], fileName, { type: 'image/png' });
-      saveImage(imageFile);
-      setValidationErrors(null);
-    }, 'image/png');
+    cropCanvas.toBlob((blob) => {
+      if (!blob) {
+        console.error('Failed to create image blob');
+        setValidationErrors(['image_upload_component.error_generate_image_failed']);
+        return;
+      }
+      try {
+        const fileName = img.id;
+        const imageFile = new File([blob], fileName, { type: 'image/png' });
+        saveImage(imageFile);
+        setValidationErrors(null);
+      } catch (e) {
+        console.error('Failed to save image', e);
+        setValidationErrors(['image_upload_component.error_generate_image_failed']);
+      }
+    }, 'image/png');

src/layout/ImageUpload/ImageControllers.tsx (2)

45-53: Fix zoom slider normalization (broken zoom and thumb position)

Utils expect 0–1, slider is 0–100. Divide input by 100; multiply value by 100.

   const handleSliderZoom = (e: React.ChangeEvent<HTMLInputElement>) => {
     const logarithmicZoomValue = normalToLogZoom({
-      value: Number.parseFloat(e.target.value),
+      value: Number.parseFloat(e.target.value) / 100,
       minZoom,
       maxZoom,
     });
@@
           <input
             id={zoomId}
             type='range'
             min='0'
             max='100'
             step='0.5'
-            value={logToNormalZoom({ value: zoom, minZoom, maxZoom })}
+            value={logToNormalZoom({ value: zoom, minZoom, maxZoom }) * 100}
             onChange={handleSliderZoom}
             className={classes.zoomSlider}
           />

Also applies to: 89-96

---

99-109: Respect readOnly and add accessible name for Reset; simplify “Change” to a real button

• Disable Save/Reset/Change/Cancel when readOnly.
• Provide aria-label for the icon-only Reset button.
• Avoid asChild with a Label; use a real Button that clicks the input ref so disabled works.
  [accessibility]

-          <Button
-            onClick={onReset}
-            variant='tertiary'
-            icon={true}
-          >
-            <ArrowUndoIcon
-              title={langAsString('image_upload_component.reset')}
-              className={classes.resetButton}
-            />
-          </Button>
+          <Button
+            onClick={onReset}
+            variant='tertiary'
+            icon
+            aria-label={langAsString('image_upload_component.button_reset')}
+            disabled={readOnly}
+          >
+            <ArrowUndoIcon className={classes.resetButton} />
+          </Button>
@@
         <Button
           onClick={onSave}
           data-size='sm'
           data-color='accent'
+          disabled={readOnly}
         >
           <Lang id='image_upload_component.button_save' />
         </Button>
@@
-        <Button
-          asChild
-          data-size='sm'
-          variant='secondary'
-          data-color='accent'
-          tabIndex={0}
-          onKeyDown={(e) => {
-            if (e.key === 'Enter' || e.key === ' ') {
-              e.preventDefault();
-              fileInputRef?.current?.click();
-            }
-          }}
-        >
-          <Label htmlFor={inputId}>
-            <UploadIcon />
-            <Lang id='image_upload_component.button_change' />
-          </Label>
-        </Button>
+        <Button
+          data-size='sm'
+          variant='secondary'
+          data-color='accent'
+          onClick={() => fileInputRef.current?.click()}
+          disabled={readOnly}
+        >
+          <UploadIcon />
+          <Lang id='image_upload_component.button_change' />
+        </Button>
         <Button
           data-size='sm'
           variant='tertiary'
           onClick={onCancel}
           data-color='accent'
+          disabled={readOnly}
         >
           <Lang id='general.cancel' />
         </Button>

Also applies to: 112-118, 128-145, 146-153

src/layout/ImageUpload/ImageUploadComponent.tsx (2)

33-36: Remove unnecessary type cast on getCropArea input

The object passed to getCropArea matches its expected params; the as CropArea cast is unnecessary and misleads types. Drop the cast.
[typescript] [As per coding guidelines]

-          cropArea={getCropArea({ width: cropWidth, height: cropHeight, type: cropShape } as CropArea)}
+          cropArea={getCropArea({ width: cropWidth, height: cropHeight, type: cropShape })}

---

21-36: Verify that Label htmlFor matches the interactive control id

Label uses htmlFor={id}, while ImageDropzone sets id={baseComponentId}. In repeating groups, these can differ (indexed id). Ensure the same id is used for both to keep label association and a11y intact (pass the actual control id down to ImageCropper/ImageDropzone if needed).

If misaligned, prefer passing the indexed id from useLabel/useIndexedId to Dropzone.

src/layout/ImageUpload/ImageControllers.tsx (1)

123-125: Align accepted file types with policy (extensions/types)

Restrict accept to the allowed types from the PR: png, jpg/jpeg, heic, webp.

-          accept='image/*'
+          accept='.png,.jpg,.jpeg,.heic,.webp,image/png,image/jpeg,image/heic,image/webp'

Please confirm HEIC support in your target browsers/environments.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/layout/ImageUpload/ImageUploadComponent.tsx (1)

35-35: Consider simplifying boolean coercion.

The !!readOnly double negation is defensive but unnecessary if readOnly is already typed as boolean | undefined and ImageCropper accepts readOnly?: boolean. Simply passing readOnly would be cleaner.

Apply this diff if the types align:

-          readOnly={!!readOnly}
+          readOnly={readOnly}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/layout/ImageUpload/ImageCropper.tsx (4)

94-116: Memory leak: Image and FileReader not cleaned up

The Image and FileReader created in handleFileUpload are not cleaned up if the component unmounts or a new file is uploaded before the previous load completes. This can cause memory leaks and stale callbacks.

Track loading resources in refs and clean them up:

+  const loadingImageRef = useRef<HTMLImageElement | null>(null);
+  const fileReaderRef = useRef<FileReader | null>(null);
+
   const handleFileUpload = (file: File) => {
     const validationErrors = validateFile(file);
     setValidationErrors(validationErrors);
     if (validationErrors.length > 0) {
       return;
     }

+    // Clean up previous loading resources
+    if (loadingImageRef.current) {
+      loadingImageRef.current.onload = null;
+      loadingImageRef.current.src = '';
+      loadingImageRef.current = null;
+    }
+    if (fileReaderRef.current) {
+      fileReaderRef.current.abort();
+      fileReaderRef.current = null;
+    }
+
     const reader = new FileReader();
+    fileReaderRef.current = reader;
     reader.onload = (event) => {
       const result = event.target?.result;

       if (typeof result === 'string') {
         const img = new Image();
+        loadingImageRef.current = img;
         img.id = file.name;
         imageTypeRef.current = file.type;
         img.onload = () => {
+          if (loadingImageRef.current === img) {
             updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img });
+            loadingImageRef.current = null;
+          }
         };
         img.src = result;
       }
     };
     reader.readAsDataURL(file);
   };
+
+  // Clean up on unmount
+  useEffect(() => {
+    return () => {
+      if (loadingImageRef.current) {
+        loadingImageRef.current.onload = null;
+        loadingImageRef.current.src = '';
+      }
+      if (fileReaderRef.current) {
+        fileReaderRef.current.abort();
+      }
+    };
+  }, []);

---

138-148: Handle toBlob failures with user feedback

The toBlob callback silently fails when blob is null, providing no user feedback. Consider adding error handling and notifying the user.

     cropCanvas.toBlob((blob) => {
       if (!blob) {
+        console.error('Failed to create image blob from canvas');
+        setValidationErrors(['image_upload_component.error_save_failed']);
         return;
       }

       const newFileName = getNewFileName({ fileName: img.id });
       const imageFile = new File([blob], newFileName, { type: IMAGE_TYPE });
       saveImage(imageFile);
       setValidationErrors(null);
     }, IMAGE_TYPE);

Add the error key to your language files:

'image_upload_component.error_save_failed': 'Failed to save the cropped image. Please try again.'

---

94-116: Clean up FileReader and Image to prevent memory leaks.

The FileReader and Image instances are not cleaned up if the component unmounts during loading or if a new file is uploaded before the previous one completes. This causes unnecessary resource consumption and stale callbacks.

Track loading state and clean up on unmount or new uploads:

+  const loadingImageRef = useRef<HTMLImageElement | null>(null);
+  const fileReaderRef = useRef<FileReader | null>(null);
+
   const handleFileUpload = (file: File) => {
     const validationErrors = validateFile(file);
     setValidationErrors(validationErrors);
     if (validationErrors.length > 0) {
       return;
     }

+    // Clean up previous loading operations
+    if (loadingImageRef.current) {
+      loadingImageRef.current.onload = null;
+      loadingImageRef.current.src = '';
+      loadingImageRef.current = null;
+    }
+    if (fileReaderRef.current) {
+      fileReaderRef.current.onload = null;
+      fileReaderRef.current = null;
+    }
+
     const reader = new FileReader();
+    fileReaderRef.current = reader;
     reader.onload = (event) => {
       const result = event.target?.result;

       if (typeof result === 'string') {
         const img = new Image();
+        loadingImageRef.current = img;
         img.id = file.name;
         imageTypeRef.current = file.type;
         img.onload = () => {
+          if (loadingImageRef.current === img) {
             updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img });
+            loadingImageRef.current = null;
+          }
         };
         img.src = result;
       }
     };
     reader.readAsDataURL(file);
   };

Add cleanup on unmount:

useEffect(() => {
  return () => {
    if (loadingImageRef.current) {
      loadingImageRef.current.onload = null;
      loadingImageRef.current.src = '';
    }
    if (fileReaderRef.current) {
      fileReaderRef.current.onload = null;
    }
  };
}, []);

---

138-148: Add error handling and user feedback for toBlob failures.

The toBlob callback doesn't handle failures beyond an early return. Users won't know if the crop operation failed.

Add error logging and consider user feedback:

     cropCanvas.toBlob((blob) => {
       if (!blob) {
+        console.error('Failed to create image blob during crop operation');
+        // TODO: Consider adding user-visible error feedback (toast/alert)
         return;
       }

       const newFileName = getNewFileName({ fileName: img.id });
       const imageFile = new File([blob], newFileName, { type: IMAGE_TYPE });
       saveImage(imageFile);
       setValidationErrors(null);
     }, IMAGE_TYPE);

src/layout/ImageUpload/imageUploadUtils.ts (6)

41-52: Use naturalWidth/naturalHeight for accurate pixel calculations

Lines 42-43 use image.width and image.height, which are CSS pixels and may not reflect the actual image dimensions. For pixel-accurate canvas calculations, use naturalWidth and naturalHeight. Also guard against zero dimensions to prevent NaN or Infinity.

Apply this diff:

 export function constrainToArea({ image, zoom, position, cropArea }: ConstrainToAreaParams): Position {
-  const scaledWidth = image.width * zoom;
-  const scaledHeight = image.height * zoom;
+  const iw = image.naturalWidth || image.width || 1;
+  const ih = image.naturalHeight || image.height || 1;
+  const scaledWidth = iw * zoom;
+  const scaledHeight = ih * zoom;

   const clampX = scaledWidth > cropArea.width ? (scaledWidth - cropArea.width) / 2 : 0;
   const clampY = scaledHeight > cropArea.height ? (scaledHeight - cropArea.height) / 2 : 0;

   const newX = Math.max(-clampX, Math.min(position.x, clampX));
   const newY = Math.max(-clampY, Math.min(position.y, clampY));

   return { x: newX, y: newY };
 }

---

61-68: Use naturalWidth/naturalHeight for accurate pixel calculations

Lines 62-63 use img.width and img.height. For pixel-accurate canvas calculations, use naturalWidth and naturalHeight.

Apply this diff:

 export const imagePlacement = ({ canvas, img, zoom, position }: ImagePlacementParams) => {
-  const scaledWidth = img.width * zoom;
-  const scaledHeight = img.height * zoom;
+  const iw = img.naturalWidth || img.width || 1;
+  const ih = img.naturalHeight || img.height || 1;
+  const scaledWidth = iw * zoom;
+  const scaledHeight = ih * zoom;
   const imgX = (canvas.width - scaledWidth) / 2 + position.x;
   const imgY = (canvas.height - scaledHeight) / 2 + position.y;

   return { imgX, imgY, scaledWidth, scaledHeight };
 };

---

105-122: Guard zoom calculations against invalid values

getLogValues doesn't validate that minZoom and maxZoom are positive. Math.log of non-positive values returns -Infinity or NaN, breaking the zoom calculations. Additionally, logToNormalZoom should clamp its result to [0, 100] to keep the slider stable.

Apply this diff:

 function getLogValues({ minZoom, maxZoom }: ZoomParams): { logScale: number; logMin: number } {
+  if (minZoom <= 0 || maxZoom <= 0) {
+    throw new Error('minZoom and maxZoom must be positive');
+  }
+  if (minZoom >= maxZoom) {
+    throw new Error('minZoom must be less than maxZoom');
+  }
   const logMin = Math.log(minZoom);
   const logMax = Math.log(maxZoom);
   return { logScale: (logMax - logMin) / 100, logMin };
 }

 export function normalToLogZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number {
   const { logScale, logMin } = getLogValues({ minZoom, maxZoom });
-  return Math.exp(logMin + logScale * value);
+  const result = Math.exp(logMin + logScale * value);
+  return Math.max(minZoom, Math.min(result, maxZoom));
 }

 export function logToNormalZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number {
   const { logScale, logMin } = getLogValues({ minZoom, maxZoom });
   if (logScale === 0) {
     return 0;
   }
-  return (Math.log(value) - logMin) / logScale;
+  const safeValue = Math.max(value, Number.EPSILON);
+  const normalized = (Math.log(safeValue) - logMin) / logScale;
+  return Math.max(0, Math.min(normalized, 100));
 }

---

125-126: Use naturalWidth/naturalHeight and guard against zero dimensions

calculateMinZoom uses img.width and img.height instead of natural dimensions. It also doesn't guard against zero dimensions, which would produce Infinity.

Apply this diff:

-export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) =>
-  Math.max(cropArea.width / img.width, cropArea.height / img.height);
+export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) => {
+  const iw = img.naturalWidth || img.width || 1;
+  const ih = img.naturalHeight || img.height || 1;
+  return Math.max(cropArea.width / iw, cropArea.height / ih);
+};

---

41-52: Use naturalWidth/naturalHeight for pixel-accurate calculations.

Lines 42-43, 62-63, and 126 use image.width/image.height instead of image.naturalWidth/image.naturalHeight. The width/height properties reflect CSS sizing, which can differ from the actual image dimensions, leading to incorrect crop calculations.

Apply this diff to use natural dimensions with zero guards:

 export function constrainToArea({ image, zoom, position, cropArea }: ConstrainToAreaParams): Position {
-  const scaledWidth = image.width * zoom;
-  const scaledHeight = image.height * zoom;
+  const iw = Math.max(1, image.naturalWidth || image.width);
+  const ih = Math.max(1, image.naturalHeight || image.height);
+  const scaledWidth = iw * zoom;
+  const scaledHeight = ih * zoom;

   const clampX = scaledWidth > cropArea.width ? (scaledWidth - cropArea.width) / 2 : 0;
   const clampY = scaledHeight > cropArea.height ? (scaledHeight - cropArea.height) / 2 : 0;

   const newX = Math.max(-clampX, Math.min(position.x, clampX));
   const newY = Math.max(-clampY, Math.min(position.y, clampY));

   return { x: newX, y: newY };
 }

 export const imagePlacement = ({ canvas, img, zoom, position }: ImagePlacementParams) => {
-  const scaledWidth = img.width * zoom;
-  const scaledHeight = img.height * zoom;
+  const iw = Math.max(1, img.naturalWidth || img.width);
+  const ih = Math.max(1, img.naturalHeight || img.height);
+  const scaledWidth = iw * zoom;
+  const scaledHeight = ih * zoom;
   const imgX = (canvas.width - scaledWidth) / 2 + position.x;
   const imgY = (canvas.height - scaledHeight) / 2 + position.y;

   return { imgX, imgY, scaledWidth, scaledHeight };
 };

-export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) =>
-  Math.max(cropArea.width / img.width, cropArea.height / img.height);
+export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) => {
+  const iw = Math.max(1, img.naturalWidth || img.width);
+  const ih = Math.max(1, img.naturalHeight || img.height);
+  return Math.max(cropArea.width / iw, cropArea.height / ih);
+};

Also applies to: 61-68, 125-126

---

105-122: Guard zoom math against invalid inputs and NaN/Infinity.

Math.log with non-positive minZoom, maxZoom, or value produces -Infinity/NaN. The normalized output should also be clamped to [0, 100] to keep the slider stable.

Apply these guards:

 function getLogValues({ minZoom, maxZoom }: ZoomParams): { logScale: number; logMin: number } {
+  if (minZoom <= 0 || maxZoom <= 0) {
+    throw new Error('minZoom and maxZoom must be positive');
+  }
+  if (minZoom === maxZoom) {
+    return { logScale: 0, logMin: Math.log(minZoom) };
+  }
   const logMin = Math.log(minZoom);
   const logMax = Math.log(maxZoom);
   return { logScale: (logMax - logMin) / 100, logMin };
 }

 export function normalToLogZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number {
   const { logScale, logMin } = getLogValues({ minZoom, maxZoom });
-  return Math.exp(logMin + logScale * value);
+  const result = Math.exp(logMin + logScale * value);
+  return Math.min(maxZoom, Math.max(minZoom, result));
 }

 export function logToNormalZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number {
   const { logScale, logMin } = getLogValues({ minZoom, maxZoom });
   if (logScale === 0) {
-    return 0;
+    return 50; // Return middle of slider when min equals max
   }
-  return (Math.log(value) - logMin) / logScale;
+  const safeValue = Math.max(Number.EPSILON, value);
+  const normalized = (Math.log(safeValue) - logMin) / logScale;
+  return Math.min(100, Math.max(0, normalized));
 }

src/layout/ImageUpload/imageUploadUtils.ts (1)

45-46: Consider extracting centering divisor as a named constant.

The value 2 appears multiple times (lines 45-46, 64-65, 74-75, 90) for centering and radius calculations. While the intent is clear in context, extracting it could improve readability per the earlier review feedback.

Based on learnings

Example:

const HALF = 2;

// Then use HALF instead of literal 2 in division operations
const clampX = scaledWidth > cropArea.width ? (scaledWidth - cropArea.width) / HALF : 0;
const imgX = (canvas.width - scaledWidth) / HALF + position.x;
const cropAreaX = (canvas.width - cropArea.width) / HALF;
ctx.arc(x + width / HALF, y + height / HALF, width / HALF, 0, Math.PI * 2);

Note: This is a minor readability enhancement and may be considered pedantic.

Also applies to: 64-65, 74-75, 90-90

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro